High — fix before merge

H1. Read/fork handlers run blocking git subprocesses directly on the tokio runtime (no spawn_blocking, no timeout)

• fork_repo — crates/gitlawb-node/src/api/repos.rs:1567-1575 invokes std::process::Command::new("git").args(["clone","--mirror",…]).output() inside the async handler. output() blocks the calling worker thread; git clone --mirror of a non-trivial repo can take minutes and there is no timeout, unlike every other git walk in this file which correctly uses tokio::task::spawn_blocking (repos.rs:67, 110, 661, 1143) or tokio::process::Command (info/refs, upload-pack, receive-pack).
• Read metadata handlers — list_commits/get_tree/get_blob call store::{resolve_head, log, ls_tree, read_file} (api/repos.rs:360-361, 397, 443, 481) which are synchronous std::process::Command::output() calls (crates/gitlawb-node/src/git/store.rs:92-231). A slow disk or large repo blocks a worker thread for seconds; with a finite worker pool a handful of concurrent /commits, /tree, /blob, or a single /fork can starve the whole runtime, including /health and /ready (same runtime in build_router).
• Impact: availability/reliability regression, not just latency. git_service_timeout_secs is bypassed on these paths.
• Fix: wrap the blocking store calls (and the fork clone) in tokio::task::spawn_blocking and bound with tokio::time::timeout(git_service_timeout_secs, …), returning AppError::Timeout.

H2. write_rate_limiter is never reaped by the background cleanup loop

• crates/gitlawb-node/src/main.rs:440-455 — the periodic task clones exactly five limiters (rate_limiter, create_ip_rate_limiter, push_rate_limiter, sync_trigger_rate_limiter, peer_write_rate_limiter) and calls .cleanup() on each. state.write_rate_limiter (declared at main.rs:329-333, wired into AppState at :404) is omitted, even though it protects the entire authenticated non-creation write surface + /graphql (server.rs:84-98, 110, 154+).
• The RateLimiter design makes the inline capacity sweep amortized and explicitly relies on the background loop for full reclamation (rate_limit.rs:36-39, 94-114). With no background sweep, write_rate_limiter only reclaims expired keys once its map hits the 200,000-key cap. Until then every distinct client IP persists as a stale Window for the full 1-hour window and beyond → steady memory growth (bounded at the cap) and a latent correctness cliff: once at cap, the amortized inline sweep becomes the only reclaimer and a distinct-IP write flood serializes behind O(max_keys) scans while legitimate new IPs get spuriously 429'd between sweeps.
• Fix: add let write_rl = state.write_rate_limiter.clone(); and write_rl.cleanup().await; inside the cleanup tick beside the other five.

Medium — should fix

M1. fork_repo / create_repo orphan on-disk repos (and a spent proof / uploaded object) on late DB failure

• fork_repo ordering (api/repos.rs:1555 → 1567 → 1585 → 1605): spend proof → git clone --mirror to disk_path → release_after_write (Tigris upload) → db.create_repo. If create_repo fails, the on-disk mirror and the already-uploaded Tigris object are left with no DB row, and the single-use iCaptcha proof is already consumed.
• create_repo (api/repos.rs:208 → 210-219 → 236): proof.consume then repo_store.init (blocking git init) happen before db.create_repo. A DB failure orphans the freshly-initialized directory. init itself is also a blocking git call on the async runtime (same class as H1).
• Fix: make disk materialization and proof spend adjacent to a guaranteed-successful DB write, and on db.create_repo failure best-effort remove_dir_all(disk_path) (and prefer deferring the Tigris upload until after the row commits).

M2. TrustedProxy IP trust is config-asserted, not network-enforced — header rotation defeats every per-IP brake

• crates/gitlawb-node/src/rate_limit.rs:198-218 (client_key): in Fly/XForwardedFor modes the limiter key is taken entirely from Fly-Client-IP or the rightmost X-Forwarded-For hop, and only falls back to the socket peer when the header is absent. There is no check that the socket peer (ConnectInfo) is actually the operator's proxy. If GITLAWB_TRUSTED_PROXY is set but the node's HTTP port is reachable directly (misconfigured firewall, exposed Fly port, second ingress, internal attacker), a client can set Fly-Client-IP: <random> / append any rightmost X-Forwarded-For hop and get a fresh bucket per request, evading the creation/write/push/sync brakes (the only thing stopping a single-source spam/push flood). The unit tests cover header parsing but never assert "peer not the proxy → key on peer."
• Fix: honor the forwarded header only when ConnectInfo peer is in a configured proxy CIDR/IP allowlist; otherwise key on the socket peer (fail closed). Add a test for "trust mode set, request from a non-proxy IP."
• Note (validation): exploitability is deployment-dependent (Fly's edge does overwrite Fly-Client-IP), so rated Medium not Blocker.

M3. purge-spam --execute deletes via remove_dir_all on an unsanitized repo_name (path traversal → arbitrary repo deletion)

• crates/gitlawb-node/src/git/store.rs:450-453 — repo_disk_path sanitizes owner_did (replace([':', '/'], "_")) but joins repo_name verbatim via format!("{repo_name}.git") with no ../slash/dot validation.
• crates/gitlawb-node/src/admin.rs:243-244 calls std::fs::remove_dir_all(store::repo_disk_path(&repos_dir, &c.owner_did, &c.name)) on that path. The fail-closed marker guard (admin.rs:128) only requires a HEAD file + objects/ dir — i.e. any existing legitimate bare repo satisfies it. API names are validated (api/repos.rs:189-197), but smart_http_repo_name explicitly notes repos are "creatable via the peer mirror path, which skips API name validation" (api/repos.rs:490-494), so a burst-DID row with a ../ name can be planted, then deleted outside repos_dir.
• Fix: validate repo_name (and re-validate owner_did) the same way the serving paths do (repo_store::validate_repo_name/validate_path_components), and assert the resolved path is canonically inside repos_dir before any remove_dir_all. Ideally make repo_disk_path itself reject unsafe names.
• Note: a destructive remove_dir_all should never run on a DB-sourced, unsanitized name. End-to-end exploitability is narrow (requires the burst DID + marker check at the traversed path), so rated Low/Medium — but it is a clear defense-in-depth defect.

M4. purge-spam TOCTOU: emptiness re-checked once as a batch snapshot, not immediately before each delete — races a live push

• crates/gitlawb-node/src/admin.rs:221-235 — partition_for_delete (:224-226) re-verifies emptiness for all candidates up front, producing to_delete; the actual delete_repo_by_id runs later in a loop (:235). Between the snapshot and a given delete, a concurrent git-receive-pack on the running node can add a ref, making the repo non-empty — yet it is still deleted (silent data loss). The comment claims it "re-verify[s] emptiness immediately before deleting," but the recheck is a single batch pass, not a per-candidate check immediately before delete_repo_by_id.
• The execute path also does not acquire the node's acquire_write advisory lock (api/repos.rs:927), so it races a live push: a push can leave orphaned Tigris objects / ref-certificate rows referencing a now-deleted repos.id, and remove_dir_all can race an in-flight receive-pack writing objects/.
• Fix: re-verify ref_count_on_disk immediately before each delete_repo_by_id (or wrap recheck+delete of each repo in a repo_store.acquire_write guard / DB transaction re-checking a ref signal). Document that the tool must be run against a quiesced node for the target DID.

M5. iCaptcha gate fails open in enforce mode when the public key cannot load

• crates/gitlawb-node/src/icaptcha.rs:302-304 — inside decide, if v.key.is_none() { return Decision::Allow; } runs before the mode match, so in Mode::Enforce a missing key (iCaptcha unreachable at startup, DNS/BGP block, bad ICAPTCHA_PUBKEY) allows every create/registration request with no proof. init() (icaptcha.rs:160-166) leaves key = None and only warns. Since the gate defaults to off, the default deployment already has no captcha; enforce-mode is now trivially bypassable by denying the key fetch, directly weakening the abuse protection this PR leans on after PoW removal.
• Fix: in enforce mode fail closed (reject) when key.is_none(); or refuse to start in enforce mode without a key. At minimum document that enforce+no-key == fail-open.

M6. purge-spam deletes the DB row, then removes the dir; a disk failure is swallowed while the deleted counter is already incremented

• crates/gitlawb-node/src/admin.rs:235-248 — on Ok(n) the row is counted as deleted before remove_dir_all, which on error only warn!s. The final "deleted N repo row(s)" summary therefore reports success even when the on-disk repo survives → DB/disk drift and a misleading operator-facing count. No retry / non-zero exit signal.
• Fix: remove the on-disk dir first (or track disk-failure separately from the DB-deleted count) and surface "N deleted, M failed."

M7. delete_repo_by_id (and purge-spam) leave dependent rows orphaned

• crates/gitlawb-node/src/db/mod.rs:1288-1294 — DELETE FROM repos WHERE id = $1 with no cascade. Child tables keyed on repo_id (branch_cids, ref_certificates, visibility_rules, repo_stars, encrypted_blobs, received_ref_updates, repo_replicas, arweave_anchors, webhooks, pull_requests) keep orphan rows that accumulate and can be re-joined if a repo with the same id is later created.
• Fix: add ON DELETE CASCADE (or explicit child deletes) for the repo_id FKs, ideally within the same transaction as the row delete.

Low / Info — nice to have

• L1. Rate-limiter bounded-map DoS. rate_limit.rs:96-114 rejects new keys when at the 200k cap; a distinct-key flood (IPv6 variation or the M2 header rotation) then 429s legitimate new clients for up to a full window. Consider evicting expired entries to admit new keys, or a metric on cap-full rejections.
• L2. Limiter state is per-process / in-memory (rate_limit.rs:24-41). In multi-machine Fly deployments the effective per-IP/per-DID limit is N × configured and a restart wipes buckets. Known tradeoff — document explicitly; consider shared counters for the spam-relevant paths.
• L3. Non-atomic create_repo. api/repos.rs:203-236 check-then-insert with no UNIQUE(owner_did, name); the idx_repos_owner_key_name is a plain index (db/mod.rs:474-490). A concurrent create race surfaces a raw UNIQUE(disk_path) violation mapped to a generic 500 instead of RepoExists (409).
• L4. Read handlers mask git errors as empty 200 OK. api/repos.rs:361 (store::log(...).unwrap_or_default()) and the get_tree siblings coerce real backend failures into empty lists, unlike get_blob (api/repos.rs:398-410) which classifies errors. Map Err to AppError::Git/NotFound.
• L5. parse_ref_updates silently tolerates malformed/truncated bodies (api/repos.rs:1651-1699) → empty Vec<RefUpdate>; receive-pack then proceeds with zero ref updates (skipping branch-protection/owner-push checks). Return 400 on a non-empty body that parses short.
• L6. Push bookkeeping errors swallowed (api/repos.rs:959, 977, 982, 999-1005 let _ = / only-warn!) — transient DB blips silently lose record_push/update_trust_score. At least raise log severity / add a durable outbox.
• L7. git_info_refs receive-pack brake skipped when client_key returns None (api/repos.rs:556 / rate_limit.rs:198): if no trusted header and no ConnectInfo peer, the if let Some(key) guard is skipped and the request proceeds unthrottled. Unreachable in prod (socket peer always present) but a divergent contract from rate_limit_by_ip; document or reject instead of skip.
• L8. Malformed rate-limit env vars silently fall back to defaults. main.rs:304-307, 325-328, 343-346 parse GITLAWB_CREATE/WRITE/PUSH_RATE_LIMIT with .ok().and_then(parse).unwrap_or(default); a fat-fingered value silently disables the intended (stricter) brake with no warning, unlike the clap-validated siblings. Warn (or fail fast) on present-but-unparseable values.
• L9. .env.example docs drift. Missing GITLAWB_IPFS_API, GITLAWB_TIGRIS_BUCKET, GITLAWB_METRICS_ADDR, GITLAWB_SHUTDOWN_GRACE_SECS (parsed in config.rs:88,161,188,194). Reverse direction is clean. Add with defaults.
• L10. Weak dry-run safety test. admin.rs:471-492 (dry_run_deletes_nothing) seeds repos with an empty repos_dir, so run_purge_spam hits the no-candidate early return before any delete logic — it does not exercise the if !execute { return } guard when candidates exist. Add a dry-run test with a real empty on-disk target repo so a regression that deletes in dry-run is caught.
• L11. Unauthenticated /metrics (main.rs:828-875, config.rs:187 warns it is unauthenticated). If bound to a non-loopback addr it leaks peer/repo/push counts. Operator-controlled; note in hardening guidance.
• L12. Git error strings returned to clients (AppError::Git(e.to_string()) sites, e.g. api/repos.rs:398-410). Can surface internal paths; return opaque messages, keep detail in server-side logs.

I found issues that need to be addressed before this is ready.

Merge readiness

• All required GitHub checks on head da876e8491fe42d276a268b95f32eb1bced1a112 are passing; branch is mergeable against current main (7ae4707c4848a4d8fab75a3edf510e5508d3a8e1).
• Prior jatmn findings on release-manifest rollback and gitlawb-core dependency-purity gate appear resolved on this head (no .release-please-manifest.json regression in the three-dot diff; workflow gate retained on main).
• Reconcile merge commit 1459cf2: it dropped durable-upload rollback code that earlier #196 commits on this branch had landed. The P1 item below is not a request to fix novel main-only debt in isolation — it asks you to re-land your own fix and align comments with behavior.

Findings

🟠 P1 — Re-land close/merge durable-upload rollback (lost in merge; comment now overclaims)

📍 Where: crates/gitlawb-node/src/api/issues.rs (close_issue), crates/gitlawb-node/src/api/pulls.rs (merge_pr), and the git helpers/tests commit 1d8c281 had added (crates/gitlawb-node/src/git/issues.rs; merge tip restore in git/store.rs for merge_pr).

💥 What fails: A local issue close or branch merge succeeds on disk, then RepoWriteGuard::release hits a definite publish refusal (UploadFailed / Fenced). The handler returns 5xx while the local tree keeps the mutation. RepoStore::acquire fast-path can serve that state until a later refresh.

// close_issue on head (comment promises rollback; implementation does not)
guard.release(close_result.is_ok()).await.into_result()?;

create_issue in the same file already compensates on refusal:

guard.release_compensating(true, move |path| {
    git_issues::delete_issue_ref(&git_bin, path, &issue_id_for_comp, deadline)
})

At merge-base 4fc041a, close_issue and merge_pr already used plain release with no compensating closure — the same observable gap. This PR is not inventing a new class of bug; it claimed to close that class on close/merge, implemented it in 1d8c281, then dropped it in merge 1459cf2 while leaving a comment at close_issue that still describes rollback.

🔎 Root cause: This branch’s merge integration removed compensating release, snapshot helpers, and RED tests, without either restoring the behavior or deleting the misleading comment. merge_pr was not re-synced after the same merge.

📜 Stated contract:

create_issue uses release_compensating on publish failure; this branch’s commits 5a46ef6 / 1d8c281 and the PR description claim improved close/merge behavior when durable storage fails; the in-diff comment at close_issue documents pre-close OID snapshot and rollback.

🏷️ Attribution: Incomplete claimed fix on this branch (merge regression of your own patch). Underlying close/merge behavior without compensation is unchanged pre-existing at merge-base — not a mandate to scope-creep into unrelated main call sites.

📌 In this PR:

• close_issue handler and the rollback comment block (api/issues.rs)
• merge_pr handler (api/pulls.rs)
• re-apply helpers and tests from 1d8c281 (issue_ref_oid, restore_issue_ref, issue_close_failed_upload_rolls_back_to_open, merge analogue)

🔒 Unchanged on main: create_issue compensating pattern, receive-pack path, and Tigris conditional-PUT design — do not redesign upload core.

🔧 Required correction: Cherry-pick/reapply the 1d8c281 shape: capture pre-mutation refs, guard.release_compensating so definite refusal runs rollback while the advisory lock is held, restore helpers and RED tests, and either implement rollback or remove the rollback comment so operator-facing text matches code. Close every listed in-diff row in one pass.

🛠️ Author fix: Re-land the fix this branch already wrote — handlers, helpers, and tests together. Do not patch only the comment or only close_issue.

🚫 Out of scope: Fixing every other write path on main that never used compensation; rebuilding Tigris versioning/conditional-PUT semantics (separate jatmn thread).

🟡 P2 — Purge execute must still delete the object-store archive when disk-path re-validation fails after the DB row is gone

📍 Where: crates/gitlawb-node/src/admin.rs — run_purge_spam execute loop after successful delete_repo_by_id.

💥 What fails: The DB row is deleted and summary.deleted increments, but validated_repo_disk_path then returns Err. The loop continues before delete_archive, leaving the archive in the bucket.

🔎 Root cause: Archive deletion is sequenced after disk-path validation; the error arm exits the iteration without calling repo_store.delete_archive.

📜 Stated contract:

Comments at the execute tail: delete the archive "else it survives and can be downloaded into a later repo created with the same owner/name"; archive_failed exists so survivors are visible.

🏷️ Attribution: PR-introduced (admin.rs is new).

📌 In this PR:

• run_purge_spam execute loop only

🔧 Required correction: On path-validation failure after a successful row delete, still attempt delete_archive (and count archive_failed on error) before continue, or reorder so archive deletion is not skipped when disk removal is refused.

🛠️ Author fix: Close this path in the same change set as any other purge execute fixes — do not fix archive-only while leaving the symlink recheck gap below unaddressed if you touch ref_count_on_disk.

🚫 Out of scope: Redesigning purge to be transactional across DB+disk+S3.

🟡 P2 — Execute-time emptiness recheck must apply the same symlink fail-closed rule as candidate selection

📍 Where: crates/gitlawb-node/src/admin.rs — ref_count_on_disk (used for under-lock recheck and partition_for_delete) vs local_refs_on_disk.

💥 What fails: Selection rejects a symlink at the repo path (symlink_metadata + warn). Between selection and under-lock recheck, the path can become a symlink to an empty bare repo outside repos_dir. ref_count_on_disk follows the link (HEAD/objects via the target), reports 0 refs, and purge deletes the DB row while path_within may refuse disk removal.

🔎 Root cause: Asymmetric guards: symlink rejection exists only on the selection closure, not on the authoritative recheck function.

📜 Stated contract:

Test symlink_to_outside_empty_bare_fails_closed and module docs: a symlink at the purge target must fail closed and must not be treated as empty-and-purgeable.

🏷️ Attribution: PR-introduced.

📌 In this PR:

• ref_count_on_disk
• under-lock recheck at execute (ref_count_on_disk call)
• add execute-path regression test mirroring the selection symlink test

🔧 Required correction: Share one helper or duplicate the symlink_metadata symlink guard from local_refs_on_disk inside ref_count_on_disk so execute cannot admit symlink targets as empty.

🛠️ Author fix: Fix selection and execute siblings together; add the execute symlink test in the same pass.

🚫 Out of scope: Changing global repo layout or symlink policy outside purge.

🔵 P3 — Keep the exhaustive limiter-sweep regression test aligned with write_rate_limiter

📍 Where: crates/gitlawb-node/src/main.rs — sweep_evicts_expired_keys_from_every_limiter.

💥 What fails: The test’s limiters array still lists ten limiters and omits write_rate_limiter, while AppState::cleanup_rate_limiters sweeps it (H2). Removing write cleanup from production would not fail this test.

📜 Stated contract:

Comment above the test: "Every per-key limiter the router mounts must be swept"; companion cleanup_rate_limiters_reaps_the_write_limiter in state.rs documents H2.

🏷️ Attribution: PR-introduced omission when write_rate_limiter was added.

🔧 Required correction: Add write_rate_limiter to the test seed list and limiters closure (mirror the state-level guard).

🛠️ Author fix: One-line-array update plus assert loop — include in the same commit as any rate-limit touch if you are already editing main.rs tests.

🚫 Out of scope: Changing sweep interval or limiter algorithms.

I found issues that need to be addressed before this is ready.

Merge readiness

• GitHub reports mergeable: CONFLICTING / mergeStateStatus: DIRTY on head 96ed2b24ff4fe04679bccde2bd4d7dbf5fca020a. Rebase onto current main, resolve conflicts, and re-run review on the resolved diff.
• The branch is at least three commits behind origin/main relative to merge-base 4fc041a3995965e199c5dc49d276717b40ab111a; security-sensitive listing/rate-limit work on main may not be present on this head until rebased.
• CI signal is thin: only CodeRabbit (skipped) on the latest check rollup. Treat merge as blocked until repo-required checks run green on the rebased head.

Findings

🟠 P1 — close_issue documents durable-upload rollback but still uses plain release

📍 Where: crates/gitlawb-node/src/api/issues.rs (close_issue).

💥 What fails: The handler closes the issue on disk, then calls guard.release(close_result.is_ok()). If the object-store publish fails, the client gets a 5xx while the local repo still shows the issue closed—same gap create_issue already closes with release_compensating.

// added in this PR — promises rollback; no snapshot or compensator follows
// Snapshot the issue ref's pre-close OID so a failed durable upload can roll
// the close back on local disk, mirroring create_issue. ...

let close_result = git_issues::close_issue(&disk_path, &issue_id);
guard.release(close_result.is_ok()).await.into_result()?;

🔎 Root cause: This diff adds operator-facing text claiming rollback “mirroring create_issue” without wiring the compensating release path that create_issue already uses.

📜 Stated contract:

In-diff comment at close_issue (this PR) requires pre-close snapshot and local rollback on failed durable upload, mirroring create_issue.

create_issue uses release_compensating + git_issues::delete_issue_ref on publish failure.

🏷️ Attribution: Incomplete claimed fix (comment introduced on this head; underlying plain release matches merge-base, but the new comment overclaims).

📌 In this PR:

• close_issue comment block and release call in api/issues.rs

🔒 Unchanged on main: Tigris publish / RepoWriteGuard core; merge_pr plain release (not in this diff—do not conscript unless you re-land branch rollback work deliberately).

🔧 Required correction: Re-land the compensating pattern (snapshot pre-close ref, release_compensating rollback under the advisory lock, regression test) or remove the rollback comment so code and docs match. Mirror the existing create_issue compensator shape; do not redesign upload core.

🛠️ Author fix: Close the root cause on every listed in-diff row in one pass—comment, release path, and test together. Do not patch only the comment.

🚫 Out of scope: Rewriting merge_pr, receive-pack, or unrelated main call sites unless you explicitly re-import the lost branch rollback commit.

🟡 P2 — Purge execute recheck omits the symlink fail-closed gate used at selection

📍 Where: crates/gitlawb-node/src/admin.rs — local_refs_on_disk vs ref_count_on_disk, execute loop authoritative recheck.

💥 What fails: Selection rejects a symlink at the repo path (fail-closed). Between selection and --execute, a symlink can point at an empty bare repo outside repos_dir. ref_count_on_disk follows the link, reports zero refs, and the row is deleted; path_within may refuse remove_dir_all, producing DB/disk drift.

🔎 Root cause: This PR hardens selection with symlink_metadata but the execute-time authoritative recheck uses ref_count_on_disk, which skips the same symlink gate.

📜 Stated contract:

local_refs_on_disk treats a symlink at the target as non-empty (fail-closed) and documents that a symlink to an outside empty bare repo must never become a candidate.

🏷️ Attribution: PR-introduced asymmetry (new execute recheck path in admin.rs).

📌 In this PR:

• ref_count_on_disk and execute-loop recheck under the per-repo lock in admin.rs

🔒 Unchanged on main: path_within containment guard; selection tests already cover symlink at selection time.

🔧 Required correction: Share the selection symlink/metadata rules in ref_count_on_disk (or call a single helper) so execute-time emptiness matches selection fail-closed behavior. Add or extend a test that fails if execute recheck follows a symlink.

🛠️ Author fix: Fix the helper and execute path in one pass; do not tighten only dry-run selection.

🚫 Out of scope: Changing EXCLUDED_DIDS / target DID constants; broader purge policy.

🔵 P3 — build_ws_schema rustdoc contradicts the wired schema

📍 Where: crates/gitlawb-node/src/graphql/mod.rs (build_ws_schema).

💥 What fails: New rustdoc says “no query or mutation root exists on this type,” but the function builds QueryRoot + EmptyMutation + SubscriptionRoot. Maintainers can underestimate WS query load (mutations are correctly blocked; queries are not).

🔎 Root cause: In-diff documentation added when splitting WS schema does not match the actual Schema::build call.

📜 Stated contract:

In-diff build_ws_schema doc (this PR) claims the WS schema cannot execute queries or mutations.

🏷️ Attribution: PR-introduced misleading operator/maintainer copy.

📌 In this PR:

• build_ws_schema module docs in graphql/mod.rs

🔒 Unchanged on main: HTTP /graphql write brake; intentional EmptyMutation on WS.

🔧 Required correction: Rewrite the rustdoc to state mutations are disabled on WS but queries still run via QueryRoot, matching GitlawbWsSchema and server comments about subscriptions-only mutations.

🛠️ Author fix: Update the in-diff doc string in the same pass; do not re-expose MutationRoot on WS unless product intent changes.

🚫 Out of scope: Adding a write brake to /graphql/ws (documented as intentional in .env.example and tests).

PR Review — Twigpine/node #196

feat(node): per-IP write-surface rate brake + purge-spam admin tool

Head: e9e86a15d41bddc9b952dbe2cbd2057c22fb0796 · Base: 55ca89bfffa2c81b7f7b6f30cee7743f000197ce

Merge readiness

• Required check test (beta) failed on run 43114804801: ipfs_pin::tests::discovery_record_elapsed_leaves_neither_row panicked. That test lives in ipfs_pin.rs, which is not in this PR’s 21-file diff; test (stable) passed on the same run. Treat as likely unrelated / flaky on beta until reproduced on a change that touches ipfs_pin, but merge state is BLOCKED until CI is green or maintainers waive beta.
• Branch is up to date with origin/main (0 behind, 69 ahead). No rebase conflict risk observed.
• Prior jatmn review threads requested write-brake coverage and purge safety; current head adds broad route tests and admin.rs guards. Remaining gaps are captured in Findings below (not a blanket re-request of older themes).

Findings

🟠 P1 — Purge execute can delete the DB row but leave the object-store archive

📍 Where: crates/gitlawb-node/src/admin.rs — run_purge_spam execute loop, after delete_repo_by_id succeeds.

💥 What fails: On the Ok(n) branch, if validated_repo_disk_path returns Err, the code increments disk_failed and continues before delete_archive runs, even though the DB row was already removed.

🔎 Root cause: Archive deletion is sequenced after disk-path validation; a continue short-circuits the whole tail of the success arm, including object-store cleanup.

📜 Stated contract:

Delete the object-store archive too… else it survives and can be downloaded into a later repo created with the same owner/name.

🏷️ Attribution: PR-introduced (admin.rs is new).

📌 In this PR:

• run_purge_spam execute path — archive delete skipped when post-delete path validation fails

🔒 Unchanged on main: N/A (new tool).

🔧 Required correction: After a successful delete_repo_by_id, still attempt delete_archive (with the same generation fence) when an object store is configured, even if local disk removal is refused; or reorder so path validation happens before the DB delete. Do not report a clean delete count when the archive was not addressed.

🛠️ Author fix: Close the purge execute lifecycle on every success arm in one pass: DB row, disk dir (when safe), and archive delete/share the same accountability rules. Adjust tests so a post-delete path-validation failure still exercises archive cleanup (or proves intentional skip with an explicit summary counter).

🚫 Out of scope: Redesigning Tigris delete semantics on main.

🟡 P2 — Purge execute summary under-reports unsettled-work skips

📍 Where: crates/gitlawb-node/src/admin.rs — PurgeSummary and final println! after --execute.

💥 What fails:

• skipped_unsettled is incremented when has_unsettled_repo_work is true, but the closing summary line never prints that counter.
• When has_unsettled_repo_work returns Err, the repo is skipped fail-closed but no summary field is incremented (unlike skipped_store_error / skipped_locked).

🔎 Root cause: Operator-facing accounting was not kept in sync with the new PurgeSummary fields and the unsettled-work guard.

📜 Stated contract:

skipped_unsettled — Candidates skipped because receive-pack or transition work still has in-flight work on the repo.

🏷️ Attribution: PR-introduced.

📌 In this PR:

• PurgeSummary struct
• execute-loop println!
• has_unsettled_repo_work error arm

🔒 Unchanged on main: N/A.

🔧 Required correction: Include skipped_unsettled in the final execute summary (and dry-run guidance if applicable). Add a dedicated counter (or reuse an existing skip bucket) for unsettled probe failures, with a regression test that forces has_unsettled_repo_work Err and asserts the summary reflects it.

🛠️ Author fix: Align every skip/fail path in the execute loop with a PurgeSummary field and the printed totals in one change.

🚫 Out of scope: Changing unsettled-work SQL semantics.

🟡 P2 — close_issue compensation does not honor git_service_timeout like create_issue

📍 Where: crates/gitlawb-node/src/api/issues.rs — close_issue compensating branch (new hunk).

💥 What fails: On successful local close, if durable upload fails, compensation calls git_issues::create_issue(path, …) with no deadline, while the sibling create_issue handler passes git_bin and a git_service_timeout_secs deadline into delete_issue_ref inside release_compensating.

🔎 Root cause: The new close path discards git_bin / deadline (let _ = (git_bin, deadline)) even though the in-diff comment claims mirroring create_issue.

📜 Stated contract:

Snapshot the issue's pre-close JSON so a failed durable upload can roll the close back on local disk, mirroring create_issue.

🏷️ Attribution: PR-introduced.

📌 In this PR:

• close_issue compensating closure

🔒 Unchanged on main: create_issue compensation pattern (use as reference).

🔧 Required correction: Apply the same timeout-bound git rewrite strategy create_issue uses (or an equivalent bounded compensation) when restoring prior_json after upload failure.

🛠️ Author fix: Make close and create compensation symmetric in one pass; add/extend a test that upload failure on close triggers bounded rollback behavior.

🚫 Out of scope: Rewriting release_compensating for all handlers.

🟡 P2 — create_repo can return 201 while the initial archive upload failed

📍 Where: crates/gitlawb-node/src/api/repos.rs — create_repo (new advisory-lock + create_upload sequence).

💥 What fails: After db.create_repo, the handler awaits the background upload with let _ = upload.await and returns 201 Created regardless of join/upload failure. Comments assert the row, disk, and archive are consistent before the lock is released.

🔎 Root cause: Upload outcome is not propagated to the HTTP response; error paths can also drop the creation lock before the upload task finishes (success path only awaits upload).

📜 Stated contract:

Keep the initial archive upload inside the creation lock… Row + on-disk dir now both exist consistently; the race window is closed.

🏷️ Attribution: PR-introduced.

📌 In this PR:

• create_repo success and error paths around create_upload / repo_lock.release()

🔒 Unchanged on main: Prior create flow without this lock/upload ordering.

🔧 Required correction: Treat a failed create-only upload like other archive failures relevant to creation (fail the request or compensating-delete row+disk), and keep the advisory lock until the upload task completes on all exit paths that inserted a row, matching the stated race comment.

🛠️ Author fix: Close create/upload/lock lifecycle on every exit path in one pass; add regression coverage for upload failure during create under Tigris.

🚫 Out of scope: Changing fork/upload semantics outside create_repo.

🔵 P3 — purge-spam cannot take GITLAWB_TIGRIS_BUCKET from CLI after the subcommand

📍 Where: crates/gitlawb-node/src/config.rs (tigris_bucket not global = true); lib.rs run_admin_command.

💥 What fails: gitlawb-node purge-spam --tigris-bucket … is rejected by clap; only repos_dir and database_url are global. Tigris-backed authoritative purge requires env or flags before purge-spam.

🔎 Root cause: Admin wiring reads config.tigris_bucket, but the CLI surface for purge does not expose it post-subcommand.

📜 Stated contract:

Build the object store so purge is authoritative against remote state… When no bucket is configured this stays None and purge is local-only.

🏷️ Attribution: PR-introduced admin entrypoint.

📌 In this PR:

• PurgeSpam subcommand
• run_admin_command object-store setup

🔒 Unchanged on main: Daemon flag layout.

🔧 Required correction: Mark tigris_bucket (and any other store fields purge needs) global = true, or document in purge-spam --help / operator runbooks that bucket config must come from env or pre-subcommand flags.

🛠️ Author fix: Pick one operator path and make help, .env.example, and clap agree in one pass.

🚫 Out of scope: New object-store providers.

I found issues that need to be addressed before this is ready.

Merge readiness

• All required checks are green on head 1187dcab79c93fb38188c50138d30db65f00e265 (stable + beta tests, fmt/clippy, build).
• Branch is 0 commits behind origin/main (merge-base 55ca89bfffa2c81b7f7b6f30cee7743f000197ce); mergeable.
• GitHub review decision is still CHANGES_REQUESTED from earlier heads; several prior jatmn items (close_issue compensating release, purge archive/symlink siblings) are fixed on this head — see findings below for what remains.

Findings

🟠 P1 — Re-land merge durable-upload rollback claimed in release notes (close path fixed; merge path not)

📍 Where: crates/gitlawb-node/src/api/pulls.rs (merge_pr).

💥 What fails: store::merge_branch mutates the bare repo, then guard.release(merge_result.is_ok()) publishes to durable storage. On definite publish refusal the handler returns 5xx, but there is no git rollback of the merge commit (unlike close_issue, which now uses release_compensating with a pre-close snapshot in api/issues.rs).

guard.release(merge_result.is_ok()).await.into_result()?;
let merge_sha = merge_result.map_err(|e| AppError::Git(e.to_string()))?;
state.db.merge_pr(&pr.id, &merger_did).await?;

🔎 Root cause: This PR advertises improved close/merge behavior on storage failure and implements compensation for close, but merge_pr still uses plain release with no compensating closure.

📜 Stated contract:

CodeRabbit release notes on this PR: "Improved pull-request merge handling when durable storage operations fail."

🏷️ Attribution: Incomplete claimed fix on this branch (close fixed; merge not). Underlying merge-without-compensation also exists at merge-base — not a mandate to refactor all main writers.

📌 In this PR:

• merge_pr handler (pulls.rs)
• any merge rollback helpers/tests this branch previously added for merge_pr but did not keep on head

🔒 Unchanged on main: RepoWriteGuard publish/quarantine core, receive_pack path, Tigris conditional PUT semantics.

🔧 Required correction: Mirror the close_issue / create_issue pattern for merge_pr: capture pre-merge refs (or merge parent), use release_compensating so definite publish refusal rewinds the local merge while the advisory lock is held, and add a RED test analogous to create_issue_rolls_back_local_ref_when_publish_refuses.

🛠️ Author fix: Land merge compensation, helpers, and tests together in one pass. Do not patch only release notes or only comments.

🚫 Out of scope: Rebuilding every other write handler on main that never compensated.

🟡 P2 — create_repo upload rollback overclaims when the DB row delete fails

📍 Where: crates/gitlawb-node/src/api/repos.rs (create_repo, initial archive upload inside the creation lock).

💥 What fails: When the empty-repo upload fails, the handler calls delete_repo_by_id, then remove_dir_all, then returns an error stating the create was rolled back. If delete_repo_by_id errors (warn-only), execution still removes the on-disk directory and returns the rollback wording.

🔎 Root cause: Rollback success is not gated on both row delete and dir removal succeeding; operator-visible text overstates recovery.

📜 Stated contract:

In-diff comment above the upload block: a failed upload must "roll the create back (row + dir)" before surfacing the error instead of reporting success.

🏷️ Attribution: PR-introduced (new upload-in-lock + rollback block on this branch).

📌 In this PR:

• create_repo upload failure path only

🔒 Unchanged on main: Proof consume ordering, iCaptcha core, unrelated handlers.

🔧 Required correction: If delete_repo_by_id fails, do not claim full rollback — return an error that states partial state (orphan row and/or orphan dir) and/or retry row delete before removing disk; add a regression test where mocked delete_repo_by_id fails.

🛠️ Author fix: Close the handler message and control flow together; do not change only the user-facing string.

🚫 Out of scope: Redesigning repo creation or Tigris upload pipeline.

🟡 P2 — Purge execute: post-commit window allows mirror ingest before remove_dir_all

📍 Where: crates/gitlawb-node/src/admin.rs (run_purge_spam execute loop); interacts with crates/gitlawb-node/src/db/mod.rs (delete_repo_by_id slug xact lock) and upsert_mirror_repo.

💥 What fails: Purge holds try_lock_repo (SHA-based session advisory lock) across delete + disk removal. delete_repo_by_id takes the slug hashtextextended transaction lock only until commit. After the row delete commits, a concurrent upsert_mirror_repo can insert a new repos row for the same normalized slug while purge still runs remove_dir_all on that owner/name path — wiping disk under a live row.

🔎 Root cause: Slug-level serialization ends at transaction commit; on-disk deletion is not serialized against mirror ingest the way the in-transaction sibling check is.

📜 Stated contract:

delete_repo_by_id comment: slug xact lock serializes purge against concurrent upsert_mirror_repo; purge module goal that DB row and on-disk dir stay consistent.

🏷️ Attribution: PR-introduced (admin.rs new; expanded cascade in this diff). DB test purge_serializes_with_mirror_ingest_via_advisory_lock holds a session slug lock for the whole scenario — live purge does not hold that lock across disk delete.

📌 In this PR:

• run_purge_spam execute loop (after successful delete_repo_by_id, before / during disk removal)
• add execute-path regression mirroring the DB race test with the real purge lock sequence

🔒 Unchanged on main: Global mirror ingest API; do not remove slug lock from upsert_mirror_repo.

🔧 Required correction: Hold slug advisory serialization from before delete through disk removal, or re-check sibling_exists / row absence immediately before remove_dir_all and abort disk delete if a new row landed.

🛠️ Author fix: Close purge execute + test in one pass.

🚫 Out of scope: Transactional purge across DB+disk+S3 in one atomic step.

🔵 P3 — Sweep integration test still omits write_rate_limiter

📍 Where: crates/gitlawb-node/src/lib.rs — sweep_evicts_expired_keys_from_every_limiter.

💥 What fails: The test seeds and asserts cleanup for ten limiters but not write_rate_limiter, even though AppState::cleanup_rate_limiters sweeps it (H2; guarded separately in state.rs).

🔎 Root cause: New limiter added in this PR without extending the cross-limiter regression array.

📜 Stated contract:

Comment above the test: "Every per-key limiter the router mounts must be swept."

🏷️ Attribution: PR-introduced omission.

📌 In this PR:

• sweep_evicts_expired_keys_from_every_limiter limiter list only

🔒 Unchanged on main: Sweep interval algorithms.

🔧 Required correction: Add state.write_rate_limiter to the seed loop and limiters closure (same pattern as cleanup_rate_limiters_reaps_the_write_limiter).

🛠️ Author fix: One-line array update in the same commit if touching limiter tests.

🚫 Out of scope: Changing production cleanup wiring (already correct in state.rs).

I found issues that need to be addressed before this is ready.

Merge readiness

Required checks are red on this head. GitHub Actions run 43142788397 reports test (stable) and test (beta) failed on cargo test, while fmt + clippy, build --release, MSRV, and Docker smoke passed. The live target branch main at merge-base 55ca89b is green on the same workflow class. The PR verification section claims cargo test --workspace is green — that does not match the required checks on 261f7ab3. Reconcile the failing test names from CI logs before merge (this is merge hygiene on this head, not a mandate to fix unrelated main CI debt).

Base is current. Merge-base equals live main (55ca89b); no stale-base rebase is required for metadata drift.

🟠 P1 — Post-delete purge must probe slug liveness before disk or archive destruction

📍 Where: crates/gitlawb-node/src/admin.rs (run_purge_spam execute loop, post-delete_repo_by_id disk removal and archive delete).

💥 What fails: After the canonical row is deleted, the pre-remove_dir_all guard calls db.get_repo_by_id(&c.id) for the purged primary key. A concurrent upsert_mirror_repo can insert a new row for the same owner/name slug with id {owner_short}/{name} (db/mod.rs) while purge still holds the session try_lock_repo lock — upsert_mirror_repo does not take that session lock, only the transaction-scoped lock_repo_slug that ends when delete_repo_by_id commits.

The inline comment names this mirror-ingest window, but the probe cannot see the new row:

// concurrent mirror ingest can insert a new row for this slug before remove_dir_all runs.
let row_gone = match db.get_repo_by_id(&c.id).await {
    Ok(None) => true,
    // ...
};

When row_gone is wrongly true, purge removes the on-disk bare repo. Archive delete still runs unconditionally afterward (delete_archive is not gated on slug absence), so object-store bytes can be removed while a live mirror repos row remains — DB / disk / archive drift and broken serve for that slug.

🔎 Root cause: Post-delete destructive steps use primary-key absence instead of slug-level liveness. The PR’s own comment describes slug-level re-check; the implementation checks only the deleted id.

📜 Stated contract:

Re-check immediately before the destructive op and abort the disk delete when a fresh row landed rather than wipe disk under a live row.

🏷️ Attribution: PR-introduced. There is no crates/gitlawb-node/src/admin.rs at merge-base 55ca89b; this execute path and guard are new in this diff.

📌 In this PR:

• run_purge_spam post-delete disk guard (get_repo_by_id)
• unconditional delete_archive after row delete
• operator dry-run / execute summary that can report disk+archive removal under a surviving slug row

🔒 Unchanged on main: In-transaction lock_repo_slug serialization between delete_repo_by_id and upsert_mirror_repo (existing DB test purge_serializes_with_mirror_ingest_via_advisory_lock). This finding is only the post-commit window after that xact lock is released.

🔧 Required correction: Before remove_dir_all and before delete_archive, treat the slug as live when db.get_repo(&c.owner_did, &c.name) (or equivalent owner-key-normalized lookup) returns any row. Skip both destructive steps when a row exists; count separately in PurgeSummary. Add a regression test racing upsert_mirror_repo after delete_repo_by_id commits but before disk removal (mirror id owner/name).

🛠️ Author fix: Close slug-level liveness on both disk and archive paths in one pass; do not patch only the get_repo_by_id call while leaving archive delete ungated.

🚫 Out of scope: Reworking peer-sync ingest to take session repo locks; changing mirror id format; fixing pre-existing ingest paths on unchanged main.

🟡 P2 — Issue write HTTP brake must not run before the INV-12 read gate

📍 Where: crates/gitlawb-node/src/server.rs (issue_write_routes + write_braked); crates/gitlawb-node/src/api/issues.rs (close_issue, create_issue_comment).

💥 What fails: issue_write_routes is wrapped with write_braked, so rate_limit_by_ip on the write bucket is the outermost middleware on:

• POST /api/v1/repos/{owner}/{repo}/issues/{id}/close
• POST /api/v1/repos/{owner}/{repo}/issues/{id}/comments

An exhausted write bucket returns 429 before authorize_repo_read runs in the handler. For callers probing private or missing repos, that breaks the documented status ordering: 429 can appear without the read gate having returned the opaque 404 used for hidden repos.

🔎 Root cause: This PR applied the shared HTTP write brake to the issue write router. The INV-12 ordering comment in close_issue (and the read-gated comment flow) predates this PR unchanged at 55ca89b; merge-base issue_write_routes had no .write_braked. The new outer middleware runs before the handler; the in-handler close_issue_rate_limiter still honors post-gate ordering but does not undo the outer brake.

📜 Stated contract:

Rate limiting runs AFTER this gate so a 429 cannot distinguish a hidden repo from a missing one (INV-12 read-denial status contract).

🏷️ Attribution: PR-activated. Merge-base issue_write_routes ended at add_auth_layers with no write_braked; head adds .write_braked(&write_ip_limiter).

📌 In this PR:

• server.rs issue_write_routes.write_braked
• close_issue (read-gated close + inner close_issue_rate_limiter)
• create_issue_comment (read-gated comment write)
• new integration tests that assert write brake on issue comment without INV-12 ordering

🔒 Unchanged on main: Other write groups (merges, stars, hooks, etc.) that are not read-gated before mutation may keep outer write_braked; do not remove their middleware or extend INV-12 refactors to those routes in this PR.

🔧 Required correction: Debit the write bucket for issue close and issue comment inside each handler immediately after authorize_repo_read (same placement as close_issue_rate_limiter), or split issue close/comment onto a router without outer write_braked while still debiting write_rate_limiter post-gate. Add regression tests: under an exhausted write bucket, a caller denied read access still receives the read-gate status (404/403), not 429, for both routes.

🛠️ Author fix: Fix ordering on both issue write routes together; do not move only close_issue while leaving comment on the outer brake.

🚫 Out of scope: Changing INV-12 behavior on unrelated read routes; redesigning global write-brake routing for the whole server; pre-existing GITLAWB_CLOSE_ISSUE_RATE_LIMIT env parsing (unchanged at merge-base).

I found issues that need to be addressed before this is ready.

Merge readiness

• Required GitHub checks test (stable) and test (beta) failed on head 662b9a3a31cbc973d4a4f728fcb5ccfe74f3b3aa (workflow run 43164147039). fmt, clippy, build, MSRV, and other gates are green.
• PR-owned CI blocker: Finding 1 below (issue_comment_is_rate_limited_by_ip) fails locally and is in this diff; fixing or removing it is required for a green cargo test on this branch.
• Pre-existing CI friction (not in ## Findings): test_support::tests::merge_pr_fails_closed_when_target_is_only_a_tag and merge_pr_advances_local_branch_shadowed_by_same_named_tag call git tag release without -m or -F; on dumb CI terminals that fails with “EDITOR unset.” The same invocation is present at merge-base 55ca89bf — not introduced by this feature diff. It can still block required checks on this head until those helpers pass -m (or CI sets a tag message). That is merge hygiene, not a request to redesign merge semantics in this PR.
• Out of this diff: If test (beta) also reports reconciliation::sweep_restores_tigris_cache_miss_and_skips_missing, that file is unchanged on this branch; treat as upstream flake or main-track follow-up, not a #196 code finding.
• Branch is mergeable against current origin/main (55ca89bfffa2c81b7f7b6f30cee7743f000197ce, 0 behind). No conflict markers in the prepared checkout.
• Prior review threads on advisory-lock pinning, purge/mirror serialization, merge/close compensating upload refusal (merge path), archive delete after row delete, symlink recheck, and limiter sweep are addressed on this head per code inspection and your recent comments; not re-opened except where evidence below still fails.
• Drift check (withdrawn from draft): Outer write_braked on star/merge routes does not violate INV-12 the way issue routes do (exhausted bucket yields the same 429 before the handler for missing and real slugs). Invalid GITLAWB_*_RATE_LIMIT parse → default with warn is intentional. Purge/mirror xact slug lock and related tests match your stated fixes.

🟠 P1 — Remove or rewrite the issue-comment router throttle test so it matches INV-12 wiring

📍 Where: crates/gitlawb-node/src/api/repos.rs — issue_comment_is_rate_limited_by_ip

💥 What fails: The test exhausts write_rate_limiter, POSTs to /api/v1/repos/someowner/somerepo/issues/1/comments without a signature, and expects 429. The router returns 401 because issue write routes deliberately omit the outer write_braked middleware; throttling runs inside create_issue_comment only after authorize_repo_read and authentication.

assertion `left == right` failed: issue-write routes must be IP-throttled by the write brake
  left: 401
 right: 429

🔎 Root cause: The test was added as a “representative REST write group” sibling to star/label routes that use write_braked, but issue comments follow the in-handler brake pattern documented for INV-12.

📜 Stated contract:

Issue write routes: an outer per-IP brake would answer 429 before authorize_repo_read could return the opaque 404 a hidden repo owes the caller (INV-12).

🏷️ Attribution: PR-introduced (this test is new in the diff).

📌 In this PR:

• crates/gitlawb-node/src/api/repos.rs — delete issue_comment_is_rate_limited_by_ip, or replace it with a test that signs/auths and asserts 429 from the handler path (mirror issues.rs an_exhausted_write_bucket_cannot_preempt_the_read_gate / existing create_issue_comment rate tests)

🔒 Unchanged on main: server.rs issue route group without outer brake; do not “fix” by attaching write_braked to issue routes.

🔧 Required correction: Drop the misleading router-only test or rewrite it to exercise in-handler debit with proper auth and read-gate ordering.

🛠️ Author fix: One pass on repos.rs tests only; keep coverage in issues.rs as the authority for issue-write throttle ordering.

🚫 Out of scope: Moving issue routes to middleware-level throttling.

🟡 P2 — Close-issue upload rollback must rewrite the resolved issue ref, not the URL prefix

📍 Where: crates/gitlawb-node/src/api/issues.rs — close_issue compensating release_compensating closure

💥 What fails: When the client closes via an 8-char prefix, git_issues::close_issue resolves to the full UUID and updates refs/gitlawb/issues/{full_id}. If durable upload then refuses, the compensator calls create_issue_bounded with the path issue_id (the prefix), creating/updating refs/gitlawb/issues/{prefix} instead of the canonical ref. The client sees 5xx while the full ref stays closed.

🔎 Root cause: Compensating rollback was added on this branch (merge-base used plain release on close with no upload compensation). The compensator copies the path segment into the git ref writer without using the same resolved id close_issue used for the mutation.

📜 Stated contract:

Handler comment: on failed upload, “rewrites the ref back to the pre-close content instead of leaving the local repo closed while the client saw 5xx.”

git/issues.rs test_close_issue_via_prefix: close-by-prefix mutates the resolved full ref.

🏷️ Attribution: PR-introduced (compensating close on close is new on this branch).

📌 In this PR:

• crates/gitlawb-node/src/api/issues.rs — compensator id passed to create_issue_bounded
• Add a regression test: close via prefix + refused publish → local ref content returns to pre-close on the full id

🔒 Unchanged on main: Tigris upload core; create_issue compensator pattern.

🔧 Required correction: Derive the resolved full issue id (from close_result or resolve_issue_id) and pass that into create_issue_bounded in the compensator.

🛠️ Author fix: Handler + one load-bearing test in the same commit; do not change unrelated write paths.

🚫 Out of scope: Prefix resolution behavior in git/issues.rs itself; compensating every other write path on main that never had rollback at merge-base.

I found issues that need to be addressed before this is ready.

Merge readiness

test (stable) failed on the latest workflow run (job log); test (beta) passed. Treat the stable failure as a merge blocker until the failing case is confirmed PR-owned or the run is green on head.

GitHub still reports mergeStateStatus: BLOCKED. fmt/clippy, build --release, MSRV, cargo audit, and Rust CodeQL passed on that run.

The PR head (43d4f59f) matches the fetched origin/main merge-base (55ca89bf) — no stale-branch conflict surfaced in this checkout.

Findings

🟡 P2 — Align in-diff operator copy with every surface that debits GITLAWB_WRITE_RATE_LIMIT

📍 Where: .env.example (new write-rate section), crates/gitlawb-node/src/lib.rs comment above write_rate_limiter, crates/gitlawb-node/src/state.rs write_rate_limiter field rustdoc, and the PR description consumer list (same omissions as .env.example).

💥 What fails: An operator sets GITLAWB_WRITE_RATE_LIMIT from the new template (or reads the PR body) and treats the bullet list as the full per-IP aggregate. On head, these also debit the shared write bucket but are missing or understated in that copy:

• POST …/issues/{id}/close — handler-level debit of write_rate_limiter new in this PR (merge-base only used close_issue_rate_limiter), plus the separate GITLAWB_CLOSE_ISSUE_RATE_LIMIT
• POST …/issues/{id}/comments — handler-level write_rate_limiter new in this PR
• Webhook POST / DELETE on write_routes — routes existed at merge-base but were unbraked; this PR puts them behind write_braked
• GET …/visibility on the same write_braked group — intentional in code (harmless read debiting write bucket) but not called out in the new env text
• state.rs rustdoc still says GraphQL MutationRoot only, while HTTP /graphql debits queries and playground GET as well (.env.example documents HTTP; field doc does not)

This is operator-contract drift on surfaces this PR newly attached to the bucket, not a request to fix pre-existing unbraked behavior on main.

🔎 Root cause: The write brake landed with a new env section and rustdocs that repeat an incomplete enumeration; the implementation and server.rs wiring are broader than the listed consumers.

📜 Stated contract:

Max non-creation write requests per client IP per hour: issue/PR comments, labels, stars, merges, protect/unprotect, replicas, visibility, tasks, bounties, profile, and all GraphQL HTTP requests.

— new .env.example section on this branch (presents itself as the consumer inventory for GITLAWB_WRITE_RATE_LIMIT)

🏷️ Attribution: PR-introduced copy drift (new env block; new write_rate_limiter debits on issue paths; webhooks newly behind write_braked). Not a pre-existing main doc bug — merge-base had no GITLAWB_WRITE_RATE_LIMIT section and did not debit the write bucket on close_issue.

📌 In this PR:

• .env.example write-rate section
• lib.rs comment above build_ip_limiter("GITLAWB_WRITE_RATE_LIMIT", …)
• state.rs write_rate_limiter field documentation
• PR description “What” bullet list (if still posted as operator guidance)
• Short note: issue close/comment use handler debits (INV-12), not outer write_braked

🔒 Unchanged on main: Bucket limits, creation/push/peer buckets, and route behavior outside this diff.

🔧 Required correction: Rewrite every in-diff operator surface to the same complete consumer list for head: add issue close, issue comments, webhooks, dual-bucket note for close, visibility GET on the write group, and HTTP GraphQL (queries + playground GET, not only mutations). Align state.rs rustdoc with .env.example on GraphQL HTTP vs WebSocket.

🛠️ Author fix: Update all listed surfaces in one pass so no sibling comment or env block still advertises the short list.

🚫 Out of scope: Retuning defaults, new env vars, or moving issue routes to router middleware (INV-12).

🟡 P2 — close_issue must not consume write_rate_limiter when only the close cap rejects

📍 Where: crates/gitlawb-node/src/api/issues.rs — close_issue; crates/gitlawb-node/src/rate_limit.rs — check / is_throttled.

💥 What fails: After the read gate:

if !state.write_rate_limiter.check(&key).await
    || !state.close_issue_rate_limiter.check(&key).await

check records a timestamp on allow. If the write bucket has capacity but GITLAWB_CLOSE_ISSUE_RATE_LIMIT is exhausted, the handler returns 429 after consuming one write slot. The IP loses shared write quota (stars, comments, GraphQL HTTP, etc.) without completing a close.

At merge-base, close_issue only called close_issue_rate_limiter.check — this cross-bucket bleed does not exist on main.

🔎 Root cause: This PR added write_rate_limiter to close_issue but chains two consuming checks with ||, so the first successful check always records even when the second rejects.

📜 Stated contract:

push traffic must keep its own bucket after close_issue is exhausted

— close_issue_rate_limit_does_not_drain_push_bucket in issues.rs (existing test that encodes cross-bucket isolation for close vs push; same product rule applies to close vs the shared write bucket)

📜 Stated contract (mechanism):

Unlike [check], it records nothing and never inserts a new key … WITHOUT perturbing the per-unit budget the consuming check maintains.

— RateLimiter::is_throttled in rate_limit.rs

🏷️ Attribution: PR-introduced (write_rate_limiter.check added in this diff only). Pre-existing close-only limiting on main is not the defect; the new write debit is.

📌 In this PR:

• close_issue handler debit logic (after authorize_repo_read, before auth work)
• New regression test: exhaust close bucket while write bucket has capacity; assert write counter/timestamp unchanged (mirror close_issue_rate_limit_does_not_drain_push_bucket)

🔒 Unchanged on main: INV-12 ordering (read gate before 429); issue routes must stay off outer write_braked.

🔧 Required correction: Use non-consuming is_throttled on both buckets to decide rejection; only call consuming check on both when the request will proceed past the rate gates (or equivalent logic with no partial debit). Add the regression test above.

🛠️ Author fix: Fix dual-bucket behavior and test in the same commit; do not merge the two limiters or add router middleware on issue routes.

🚫 Out of scope: Changing GITLAWB_CLOSE_ISSUE_RATE_LIMIT parsing (unchanged vs base), other write routes, or framework-wide rate-limit redesign.

I rechecked head 4b483a35 after your Nov 12 fixes. The prior P1 block (close/merge compensating publish, purge archive/disk/slug-live, close dual-bucket probes, sweep test) looks landed; deny harness 53/53 passes locally. Two low-severity items remain on this PR’s write-brake work.

Merge readiness

• GitHub still shows CHANGES_REQUESTED on this PR until a fresh approving review lands on 4b483a35.
• Required Rust jobs on the latest run are green (fmt, clippy, MSRV, release build, deny-related unit surface); test (stable) / test (beta) were still pending at review time — confirm they finish green on 4b483a35 (you noted an out-of-diff reconciliation flake on the prior head).
• Branch is mergeable with current main; no conflict blocker observed.
• Slug-normalization / mirror-ingest follow-ups you deferred to a dedicated issue are acknowledged and are not re-listed here as PR-scope defects.

Findings

🔵 P3 — close_issue can still debit write_rate_limiter without closing when both buckets look open

📍 Where: crates/gitlawb-node/src/api/issues.rs (close_issue, ~298–306)

💥 What fails: After both is_throttled probes pass, write_rate_limiter.check runs before close_issue_rate_limiter.check. If the write check succeeds and the close check then fails (close already at cap due to a concurrent close), the write slot is consumed but no close runs.

🔎 Root cause: Sequential consuming checks after non-consuming probes; the second failure does not roll back the first debit.

📜 Stated contract:

In-handler comment: a close refused by either cap spends nothing from the other bucket.

🏷️ Attribution: PR-introduced (dual-bucket + probe pattern on this branch).

📌 In this PR:

• close_issue rate-limit gate only

🔒 Unchanged on main: Outer write_braked placement on other routes.

🔧 Required correction: Debit both buckets only when both checks will succeed (e.g. paired non-consuming admission then paired check, or restore write slot if close check fails), and extend tests beyond the fully-saturated-bucket case to cover write-ok/close-fail ordering.

🛠️ Author fix: Keep INV-12 read-gate-first ordering; fix only the dual-bucket debit logic and tests.

🚫 Out of scope: Re-mounting issue routes with router-level write_braked.

🔵 P3 — New in-handler write_rate_limiter debits omit Retry-After while middleware 429s advertise window delay

📍 Where: crates/gitlawb-node/src/api/issues.rs (create_issue_comment ~185–190; close_issue write-bucket arms ~298–306)

💥 What fails: These paths return AppError::TooManyRequests with no retry-after header. This PR’s router brakes use check_retry and attach window-derived Retry-After on 429.

🔎 Root cause: Handler paths call check() (bool) instead of check_retry, and TooManyRequests maps to status-only.

📜 Stated contract:

This PR extended limiter and middleware 429 behavior to return and test window-derived Retry-After (rate_limit.rs, receive-pack advert and middleware tests).

🏷️ Attribution: PR-introduced incomplete — only the write_rate_limiter debits added or wired on this branch for issue comment/close; the pre-existing close_issue_rate_limiter-only path at merge-base already omitted Retry-After and is not part of this finding.

📌 In this PR:

• create_issue_comment post-read-gate write debit
• close_issue shared write-bucket debit (not the close-only cap alone)

🔒 Unchanged on main: Historical handler 429 sites that never debited write_rate_limiter; global redesign of every TooManyRequests variant.

🔧 Required correction: On those write-bucket rejections, use check_retry (or return the same too_many_requests(Duration) helper middleware uses) so 429 carries retry-after; one regression on comment or close write-cap is enough.

🛠️ Author fix: Local helper is fine; do not retrofit unrelated handlers in this PR.

🚫 Out of scope: Adding Retry-After to merge-base close-cap-only 429s or other untouched limiters.

🟢 Thanks for the contribution. I do not see any actionable issues from my review.

Merge readiness

Head 11cadb938260dc8d6ddd2310e3930014a4e5ddb0 is mergeable but two commits behind live origin/main (5a7576b682e1fc2566c25b09a1b69edf85e836cb). Rebase onto current main before merge so any recent main fixes ride with this branch.

Required test (stable) and test (beta) failed on run 43337356159:

• reconciliation::tests::sweep_persists_last_attempted_not_planned_tail_on_mid_batch_narrow (stable)
• ipfs_pin::tests::discovery_record_elapsed_leaves_neither_row (beta)

Neither crates/gitlawb-node/src/reconciliation.rs nor crates/gitlawb-node/src/ipfs_pin.rs appears in this PR’s three-dot diff. Treat these as required-check failures that must be green on the PR head (re-run after rebase, or chase flake/regression on main if they reproduce there). Other required checks on that run (fmt+clippy, deny harness job aside from the single beta failure, MSRV, docker smoke, CodeQL matrix) were green.

Prior jatmn review items from 2026-11-11 (close/merge compensating upload rollback, purge archive-after-disk-refusal, execute-time symlink recheck, write-limiter sweep regression) are addressed on current head per code inspection and the author’s follow-up commits through 11cadb93 (including write-bucket Retry-After on in-handler refusals and close-cap refund for the single-request dual-bucket window).